Skip to content

pod logs enhancements: option to color logs#13490

Merged
openshift-merge-robot merged 1 commit into
podman-container-tools:mainfrom
gcalin:13266
Apr 4, 2022
Merged

pod logs enhancements: option to color logs#13490
openshift-merge-robot merged 1 commit into
podman-container-tools:mainfrom
gcalin:13266

Conversation

@kbaran1998

@kbaran1998 kbaran1998 commented Mar 11, 2022

Copy link
Copy Markdown

Created an option to colourise pod logs with an option --color. You can recreate with the following steps:

  1. Create a pod with containers
bin/podman pod create --name=pod_testlogs; bin/podman run --name=testlogs_loop1_1 -d --pod=pod_testlogs busybox /bin/sh -c 'for i in `seq 1 10000`; do echo "loop1: $i"; sleep 1; done'; bin/podman run --name=testlogs_loop2_1 -d --pod=pod_testlogs busybox /bin/sh -c 'for i in `seq 1 10000`; do echo "loop2: $i"; sleep 3; done';
  1. Logs with colour:
bin/podman pod logs --tail=10 -f --color pod_testlogs
  1. Kill all pods and remove containers:
bin/podman kill --all; bin/podman rm --all; bin/podman pod rm --all

Closes #13266

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2022
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2022
Comment thread cmd/podman/containers/logs.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra blank lines.

Comment thread cmd/podman/pods/logs.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra blank lines.

Comment thread cmd/podman/pods/logs.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obove the option is --colors?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why plural? I think just --color would be even better.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we make it a string so in future we can extend it if needed?

e.g. GNU diff and ls have --color={never,always,auto} where --color=auto detects if the stdout is a tty before turning on colors.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also extend a bool in the future, it is easy to change it to a string while still keeping the --color only syntax.

Comment thread libpod/container_log.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use Colors and option.Colors

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2022
@rhatdan

rhatdan commented Mar 13, 2022

Copy link
Copy Markdown
Contributor

Please rebase and force push your commit. There should only be one commit.

@openshift-ci openshift-ci Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 16, 2022
@kbaran1998 kbaran1998 requested a review from rhatdan March 16, 2022 15:28
@rhatdan

rhatdan commented Mar 17, 2022

Copy link
Copy Markdown
Contributor

Your PR is still includes many old PRs please rebase and only submit your changes.

@keonchennl keonchennl force-pushed the 13266 branch 9 times, most recently from c5e16ee to 9a523ab Compare March 18, 2022 16:35
@keonchennl keonchennl force-pushed the 13266 branch 2 times, most recently from f2689ec to c62587e Compare March 18, 2022 16:55
@keonchennl keonchennl force-pushed the 13266 branch 2 times, most recently from 5b4c683 to 283fd60 Compare March 18, 2022 17:15
@openshift-ci openshift-ci Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 18, 2022
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2022
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2022
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2022
@kbaran1998 kbaran1998 force-pushed the 13266 branch 5 times, most recently from 80d9a4d to a54a783 Compare March 24, 2022 10:38
@openshift-ci openshift-ci Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 24, 2022
@openshift-ci openshift-ci Bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 24, 2022
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2022
@rhatdan

rhatdan commented Mar 24, 2022

Copy link
Copy Markdown
Contributor

You need to do something like

git pull origin main
git rebase -i origin
Remove all of the code in the rebase that you did not add and squash all of your commits into a single PR
then do a
git push --force

@kbaran1998

kbaran1998 commented Mar 24, 2022

Copy link
Copy Markdown
Author

@rhatdan Whenever I squash only what I added into 1 commit, I get the 884 changes and whenever I rebase, I get back 7 commits...

And if I follow instructions in DCO from the line:
To add your Signed-off-by line to every commit in this branch:

I will get back to 7 commits and 13 changes.

I seem to be going in a loop.

@mheon

mheon commented Mar 24, 2022

Copy link
Copy Markdown
Contributor

I recommend git fetch to fetch the changes, and then a git rebase upstream/main (where upstream points to this repo, not your fork) to apply your changes on top of latest main

@rhatdan

rhatdan commented Mar 24, 2022

Copy link
Copy Markdown
Contributor

Do
git pull upstream main
And then
git rebase -i origin
And see if that clears it up

@rhatdan

rhatdan commented Mar 25, 2022

Copy link
Copy Markdown
Contributor

Almost there.

Now cleanup your commit message. removing the excess text.

git commit -a --amend -s
git push --force

Should do it, and then the DCO should pass.

Comment thread cmd/podman/containers/logs.go Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
podman container logs --color --names ctrID1 ctrID2
podman container logs --color --names ctrID1 ctrID2

@kbaran1998

kbaran1998 commented Mar 28, 2022

Copy link
Copy Markdown
Author

For some reason, a test in test/e2e/run_test.go called podman run user capabilities test (line 467) is failing on the pipeline and locally, and I am not sure why. Specifically, it is giving me this output:

time="2022-03-28T20:30:41+02:00" level=warning msg="\"/\" is not a shared mount, this could cause issues or missing mounts with rootless containers"
CapBnd: 00000000a80425fb
CapEff: 0000000000000000
CapInh: 0000000000000000
CapBnd: 00000000a80425fb
CapEff: 00000000a80425fb
CapInh: 00000000a80425fb
CapBnd: 00000000a80425fb
CapEff: 00000000a80425fb
CapAmb: 0000000000000002
CapInh: 0000000000000002
CapAmb: 0000000000000000
CapAmb: 0000000000000000
CapInh: 00000000a80425fb
CapAmb: 0000000000000002
CapInh: 0000000000000000

------------------------------
• Failure [17.948 seconds]
Podman run
/usr/local/go/src/github.com/containers/podman/test/e2e/run_test.go:25
  podman run user capabilities test [It]
  /usr/local/go/src/github.com/containers/podman/test/e2e/run_test.go:467

  Expected
      <string>: CapInh: 0000000000000000
  to contain substring
      <string>: 0000000000000002

  /usr/local/go/src/github.com/containers/podman/test/e2e/run_test.go:553
------------------------------
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSintegration timing results
podman run user capabilities test               17.947786
"unlinkat /tmp/podman/imagecachedir/vfs/dir/2fa52ae883433bc788d7fa1737ca50d2802e0e39d71cae30b4c49378c1207a17/home: permission denied"


Summarizing 1 Failure:

[Fail] Podman run [It] podman run user capabilities test 
/usr/local/go/src/github.com/containers/podman/test/e2e/run_test.go:553

Ran 1 of 114 Specs in 41.103 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 113 Skipped
--- FAIL: TestLibpod (41.12s)
FAIL
FAIL    command-line-arguments  41.131s
FAIL

I used this command to recreate the error:

GOPATH=~/go go test -v test/e2e/libpod_suite_test.go test/e2e/common_test.go test/e2e/config.go test/e2e/config_amd64.go test/e2e/run_test.go

Would anyone know what could possibly be causing this?

@rhatdan

rhatdan commented Mar 28, 2022

Copy link
Copy Markdown
Contributor

You need to rebase your PR.

git pull origin main
git rebase -i origin
git push --force

@TomSweeneyRedHat

Copy link
Copy Markdown
Contributor

@kbaran1998 you're still fighting DCO issues, looks like two of your commits are unsigned. Please see: https://github.com/containers/podman/pull/13490/checks?check_run_id=5727855035

Signed-off-by: Krzysztof Baran <krysbaran@gmail.com>
Signed-off-by: gcalin <caling@protonmail.com>
@TomSweeneyRedHat

Copy link
Copy Markdown
Contributor

LGTM
and happy green test buttons
Thanks @kbaran1998 !

@rhatdan

rhatdan commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

Thanks @kbaran1998
/lgtm
/approve

@openshift-ci

openshift-ci Bot commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kbaran1998, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

podman pod logs enhancements: option to color logs

8 participants